Mute muted users (Chris's revision, 2 of 2) #1561

chrisbobbe · 2025-06-10T20:51:35Z

Stacked on #1560.

Many thanks again to @sm-sayedi for your work on this! As with #1560, this is meant to take #1429 across the finish line in time for the upcoming app launch. Thanks again for making sure I had your latest revision of #1429 before you left for vacation. 🙂

In these new commits:

In "Direct messages" (the recent-DMs page), we exclude conversations where everyone is muted.
In "Combined feed", "Starred", and "Mentions", we exclude messages in those conversations.
In the typing-status text ("Chris Bobbe is typing…"), we exclude muted users.

Fixes: #296

chrisbobbe · 2025-06-12T19:39:32Z

Revision pushed; this one also excludes muted users from the new new-DM UI, which landed after my last revision.

gnprice

Thanks! I've read so far the first three commits:
8af150c recent-dms: Exclude DM conversations where all other recipients are muted
06937e2 msglist [nfc]: Place _allMessagesVisible right after _messageVisible
8339c64 msglist [nfc]: s/VisibilityEffect/UserTopicVisibilityEffect/

and the next one except for its tests:
898a2ad msglist: In combined/mentions/starred, exclude DMs if all recipients muted

Generally all looks good; a few small comments below.

That leaves four more commits that I haven't read (though I've skimmed them):
619eb24 autocomplete: Exclude muted users from user-mention autocomplete
87df780 msglist [nfc]: Represent nobody-is-typing as null, not empty list
c40cb89 msglist: Exclude muted users from typing-status text
0acc63b new-dm: Exclude muted users

gnprice · 2025-06-12T23:39:45Z

lib/model/channel.dart

-      (false, true) => VisibilityEffect.unmuted,
-      (true, false) => VisibilityEffect.muted,
-      _             => VisibilityEffect.none,
+      (false, true) => UserTopicVisibilityEffect.unmuted,
+      (true, false) => UserTopicVisibilityEffect.muted,
+      _             => UserTopicVisibilityEffect.none,


This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it.

Yeah, agreed.

Some of the verbosity (like in these lines) will at least get resolved by the upcoming Dart feature of "dot shorthands", expected later this year:
https://github.com/dart-lang/language/blob/main/working/3616%20-%20enum%20value%20shorthand/proposal-simple-lrhn.md

I believe with that feature we'll be able to say just .unmuted, .muted, .none here, without the name of the enum type.

gnprice · 2025-06-12T23:44:02Z

lib/model/message_list.dart

+            !store.shouldMuteDmConversation(DmNarrow(
+              allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),


Let's make another DmNarrow constructor for this:

Suggested change

!store.shouldMuteDmConversation(DmNarrow(

allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),

!store.shouldMuteDmConversation(DmNarrow.ofConversation(

message.conversation, selfUserId: store.selfUserId)),

Compare the comment at the top of that class:

// Zulip has many ways of representing a DM conversation; for example code // handling many of them, see zulip-mobile:src/utils/recipient.js . // Please add more constructors and getters here to handle any of those // as we turn out to need them. class DmNarrow extends Narrow implements SendableNarrow {

It'd be bad if this logic did something inefficient like copy the list of recipient IDs, because we're doing this for potentially a large fraction of the messages in this feed (if a user uses lots of DMs). I believe this in fact doesn't make such a copy — and a constructor on DmNarrow is a good place to centralize coming to that conclusion.

Huh…it looks like DmNarrow.ofMessage, though, does copy the list of recipient IDs:

factory DmNarrow.ofMessage(MessageBase<DmConversation> message, { required int selfUserId, }) { return DmNarrow( allRecipientIds: List.unmodifiable(message.conversation.allRecipientIds), selfUserId: selfUserId, ); }

Hmm indeed.

It looks like we don't currently call that one in any place that's as performance-sensitive as this. But it's a bit of a pitfall lying around.

gnprice · 2025-06-12T23:48:04Z

lib/model/message_list.dart

+    switch (narrow) {
+      case CombinedFeedNarrow():
+      case ChannelNarrow():
      case MentionsNarrow():
      case StarredMessagesNarrow():
+        return false;
+
+      case TopicNarrow():
+      case DmNarrow():
        return true;


nit: let's keep these cases in the same order as in _messageVisible, for ease of comparing the logic. It's important that the two align (that's this method's job); and it's potentially a bit subtle to confirm that, so it's good to make it as easy as possible.

It's fine for this to have two separate groups of cases that return the same thing.

gnprice · 2025-06-12T23:48:46Z

lib/model/message_list.dart

+  MutedUsersVisibilityEffect _mutedUsersEventCanAffectVisibility(MutedUsersEvent event) {
+    switch(narrow) {
      case CombinedFeedNarrow():


similarly, put the cases in the same order as in _messageVisible

gnprice · 2025-06-12T23:52:10Z

lib/model/user.dart

+    final union = setUnion(sortedOld, sortedNew);
+
+    final willMuteSome = sortedOld.length < union.length;
+    final willUnmuteSome = sortedNew.length < union.length;


Neat, yeah, this logic works.

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

chrisbobbe · 2025-06-13T02:15:18Z

Thanks! Revision pushed.

gnprice · 2025-06-13T02:49:32Z

Thanks! Those changes look good.

There's a CI failure in build_runner.

(And there are parts of the branch I still need to read, per #1561 (review) .)

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

chrisbobbe · 2025-06-13T02:51:57Z

I just rebased atop current main (after the dependency-upgrades PR), in case that fixes CI. 🤔

Like userDisplayName does. And remove a null-check `store.getUser(userId)!` at one of the callers... I think that's *probably* NFC, for the reason given in a comment ("must exist because UserMentionAutocompleteResult"). But it's possible this is actually a small bugfix involving a rare race involving our batch-processing of autocomplete results. Related: zulip#716

Related: zulip#716

We've now centralized on store.userDisplayName and store.senderDisplayName for all the code that's responsible for showing a user's name on the screen, except for a few places we use `User.fullName` for (a) the self-user and (b) to create an @-mention for the compose box. The "(unknown user)" and upcoming "Muted user" placeholders aren't needed for (a) or (b).

@chris

(Done by adding an is-muted condition in store.userDisplayName and store.senderDisplayName, with an opt-out param.) If Chris is muted, we'll now show "Muted user" where before we would show "Chris Bobbe", in the following places: - Message-list page: - DM-narrow app bar. - DM recipient headers. - The sender row on messages. This and message content will get more treatment in a separate commit. - Emoji-reaction chips on messages. - Typing indicators ("Muted user is typing…"), but we'll be excluding muted users, coming up in a separate commit. - Voter names in poll messages. - DM items in the Inbox page. (Messages from muted users are automatically marked as read, but they can end up in the inbox if you un-mark them as read.) - The new-DM sheet, but we'll be excluding muted users, coming up in a separate commit. - @-mention autocomplete, but we'll be excluding muted users, coming up in a separate commit. - Items in the Direct messages ("recent DMs") page. We'll be excluding DMs where everyone is muted, coming up in a separate commit. - User items in custom profile fields. We *don't* do this replacement in the following places, i.e., we'll still show "Chris Bobbe" if Chris is muted: - Sender name in the header of the lightbox page. (This follows web.) - The "hint text" for the compose box in a DM narrow: it will still say "Message @chris Bobbe", not "Message @muted user". (This follows web.) - The user's name at the top of the Profile page. - We won't generate malformed @-mention syntax like @_**Muted user|13313**. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

We're free to rename these because they don't correspond to named variables in the Figma. These more general names will be used for an avatar placeholder for muted users, coming up. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

…able (Done by adding an is-muted condition in Avatar and AvatarImage, with an opt-out param. Similar to how we handled users' names in a recent commit.) If a user is muted, we'll now show a placeholder where before we would have shown their real avatar, in the following places: - The sender row on messages in the message list. This and message content will get more treatment in a separate commit. - @-mention autocomplete, but we'll be excluding muted users, coming up in a separate commit. - User items in custom profile fields. - 1:1 DM items in the Direct messages ("recent DMs") page. But we'll be excluding those items there, coming up in a separate commit. We *don't* do this replacement in the following places, i.e., we'll still show the real avatar: - The header of the lightbox page. (This follows web.) - The big avatar at the top of the profile page. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

…erRow No-op because the child Flexible -> GestureDetector -> Row has the default MainAxisSize.max, filling the available space, leaving none that would be controlled by spaceBetween.

For muted-users, coming up. This was ad hoc for mobile, for the "Reveal message" button on a message from a muted sender: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50786&m=dev

For muted-users, coming up. This is consistent with the ad hoc design for muted-users, but also consistent with the component in "Zulip Web UI Kit". (Modulo the TODO for changing icon-to-label gap from 8px to 6px; that's tricky with the Material widget we're working with.)

… type For muted-users, coming up. Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50795&m=dev

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6089-28385&t=28DdYiTs6fXWR9ua-0 Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

…uted Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

This is solely for a better order.

This is awkwardly verbose, but we're about to add another kind of visibility effect, and I think the code will end up clearer if we make a separate enum for it. Greg points out that an upcoming Dart feature "dot shorthands" should help with this verbosity: zulip#1561 (comment)

…muted In the future, this should apply to the ReactionsNarrow from the Web app too, once we have it. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

Just to skip the bit of extra work of creating new lists.

Following web: zulip#1429 (review)

This completes the planned work for muting muted users (hooray!). Fixes: zulip#296

gnprice

OK, and read those remaining 4 commits. Just small comments below.

gnprice · 2025-06-19T01:00:11Z

lib/model/autocomplete.dart

    // TODO(#236) test email too, not just name
    if (!user.isActive) return false;
+    if (store.isUserMuted(user.userId)) return false;

    return _testName(user, cache);


nit:

Suggested change

// TODO(#236) test email too, not just name

if (!user.isActive) return false;

if (store.isUserMuted(user.userId)) return false;

return _testName(user, cache);

if (!user.isActive) return false;

if (store.isUserMuted(user.userId)) return false;

// TODO(#236) test email too, not just name

return _testName(user, cache);

The comment was already a bit separated from what it applied to, so perhaps should have been moved down there sooner; but this would widen the separation, so let's fix it.

(Can stay in the same commit, no need to make a separate prep commit.)

gnprice · 2025-06-19T01:06:41Z

lib/widgets/message_list.dart

    final typistIds = model!.typistIdsInNarrow(narrow);
-    if (typistIds.isEmpty) return const SizedBox();
-    final text = switch (typistIds.length) {
+    if (typistIds == null) return const SizedBox();


msglist [nfc]: Represent nobody-is-typing as null, not empty list Just to skip the bit of extra work of creating new lists.

Hmm, this feels like the API gets a bit more complicated from the caller's perspective. What if typistIds is empty, and why is there a different possibility which is that it's null?

(In reality I think it's never empty after this change — _removeTypist removes the sub-maps when empty. But that's not obvious from the outside.)

gnprice · 2025-06-19T01:07:45Z

lib/model/typing_status.dart

-  Iterable<int> typistIdsInNarrow(SendableNarrow narrow) =>
-    _timerMapsByNarrow[narrow]?.keys ?? [];
+  Iterable<int>? typistIdsInNarrow(SendableNarrow narrow) =>
+    _timerMapsByNarrow[narrow]?.keys;


An alternate way of not creating new lists each time:

Iterable<int> typistIdsInNarrow(SendableNarrow narrow) => _timerMapsByNarrow[narrow]?.keys ?? const [];

gnprice · 2025-06-19T01:08:30Z

lib/widgets/message_list.dart

+    final filteredTypistIds = typistIds
+      .whereNot((userId) => store.isUserMuted(userId));


nit: can use a tear-off, which is shorter and I think here a bit easier to read:

Suggested change

final filteredTypistIds = typistIds

.whereNot((userId) => store.isUserMuted(userId));

final filteredTypistIds = typistIds.whereNot(store.isUserMuted);

gnprice · 2025-06-19T01:11:56Z

test/widgets/message_list_test.dart

          expected: 'Third User and Fourth User are typing…');
+        await store.setMutedUsers([eg.thirdUser.userId]);
+        await tester.pump();
+        check(tester.widget<Text>(finder)).data.equals('Fourth User is typing…');


Let's put this in a separate test case. This one (or this pair of them) is pretty long and a bit unfocused already.

The new test case can be for just a topic narrow; I don't think it needs to exercise both kinds of conversations.

gnprice · 2025-06-19T01:12:55Z

lib/widgets/new_dm_sheet.dart

+    final sansMuted = store.allUsers
+      .whereNot((User user) => store.isUserMuted(user.userId));


nit: omit boring types

Suggested change

final sansMuted = store.allUsers

.whereNot((User user) => store.isUserMuted(user.userId));

final sansMuted = store.allUsers

.whereNot((user) => store.isUserMuted(user.userId));

chrisbobbe requested a review from gnprice June 10, 2025 20:51

chrisbobbe assigned gnprice Jun 10, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 10, 2025

chrisbobbe mentioned this pull request Jun 12, 2025

Distinguish muted users in UI #1429

Closed

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from 9c82301 to 0acc63b Compare June 12, 2025 19:39

gnprice reviewed Jun 13, 2025

View reviewed changes

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from 0acc63b to ceb4e0f Compare June 13, 2025 02:15

chrisbobbe force-pushed the pr-chris-muted-users-2 branch from ceb4e0f to f3f4df2 Compare June 13, 2025 02:51

chrisbobbe and others added 16 commits June 12, 2025 23:25

lightbox: Use senderDisplayName for sender's name

9e99dea

Related: zulip#716

compose: Fix error on quote-and-replying message from unknown sender

9e8c892

Related: zulip#716

msglist [nfc]: Remove a no-op MainAxisAlignment.spaceBetween in _Send…

becfbce

…erRow No-op because the child Flexible -> GestureDetector -> Row has the default MainAxisSize.max, filling the available space, leaving none that would be controlled by spaceBetween.

button [nfc]: Have ZulipWebUiKitButton support a smaller, ad hoc size

004c05d

For muted-users, coming up. This was ad hoc for mobile, for the "Reveal message" button on a message from a muted sender: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50786&m=dev

button [nfc]: Have ZulipWebUiKitButton support ad hoc minimal-neutral…

6489bd5

… type For muted-users, coming up. Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6092-50795&m=dev

msglist: Hide content of muted messages, with a "Reveal message" button

9cd2972

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6089-28385&t=28DdYiTs6fXWR9ua-0 Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

recent-dms: Exclude DM conversations where all other recipients are m…

7b5250d

…uted Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

msglist [nfc]: Place _allMessagesVisible right after _messageVisible

c03987a

This is solely for a better order.

msglist: In combined/mentions/starred, exclude DMs if all recipients …

f23302b

…muted In the future, this should apply to the ReactionsNarrow from the Web app too, once we have it. Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

chrisbobbe and others added 4 commits June 12, 2025 23:26

autocomplete: Exclude muted users from user-mention autocomplete

d1895b7

Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

msglist [nfc]: Represent nobody-is-typing as null, not empty list

c447d4f

Just to skip the bit of extra work of creating new lists.

msglist: Exclude muted users from typing-status text

411df89

Following web: zulip#1429 (review)

new-dm: Exclude muted users

5438a65

This completes the planned work for muting muted users (hooray!). Fixes: zulip#296

gnprice force-pushed the pr-chris-muted-users-2 branch from f3f4df2 to 5438a65 Compare June 13, 2025 06:26

gnprice mentioned this pull request Jun 17, 2025

Mute muted users #296

Open

gnprice reviewed Jun 19, 2025

View reviewed changes

		!store.shouldMuteDmConversation(DmNarrow(
		allRecipientIds: allRecipientIds, selfUserId: store.selfUserId)),

		final filteredTypistIds = typistIds
		.whereNot((userId) => store.isUserMuted(userId));

	final filteredTypistIds = typistIds
	.whereNot((userId) => store.isUserMuted(userId));
	final filteredTypistIds = typistIds.whereNot(store.isUserMuted);

		final sansMuted = store.allUsers
		.whereNot((User user) => store.isUserMuted(user.userId));

Mute muted users (Chris's revision, 2 of 2) #1561

Are you sure you want to change the base?

Mute muted users (Chris's revision, 2 of 2) #1561

Conversation

chrisbobbe commented Jun 10, 2025

Uh oh!

chrisbobbe commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 13, 2025

Uh oh!

gnprice commented Jun 13, 2025

Uh oh!

chrisbobbe commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe commented Jun 12, 2025 •

edited

Loading

chrisbobbe commented Jun 13, 2025 •

edited

Loading